Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move embedded haproxy process to a distinct pid group #1136

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Move embedded haproxy process to a distinct pid group #1136

merged 1 commit into from
Jun 13, 2024

Conversation

jcmoraisjr
Copy link
Owner

The embedded haproxy process is being created in the same pid group of the controller, which makes it receive the same signals received by the controller when running under a process supervisor. This makes haproxy be prematurely terminated when the controller is still shutting down.

@jcmoraisjr
Copy link
Owner Author

#1028

@jcmoraisjr
move embedded haproxy process to a distinct pid group
d8bc567 
The embedded haproxy process is being created in the same pid group of
the controller, which makes it receive the same signals received by the
controller when running under a process supervisor. This makes haproxy
be prematurely terminated when the controller is still shutting down.
@jcmoraisjr jcmoraisjr merged commit d0deb9d into master Jun 13, 2024
2 checks passed
@jcmoraisjr jcmoraisjr deleted the jm-pid-group branch June 13, 2024 21:22
@avmnu-sng
Copy link

avmnu-sng commented Jun 15, 2024 •
edited
Loading

@jcmoraisjr Don't we need to update haproxy-shutdown.sh to send USR1, it still sends TERM which is hard stop.

haproxy-ingress/rootfs/haproxy-shutdown.sh

Line 23 in 914b581

kill $pid

kill -USR1 $pid

Even for the master-worker mode.

haproxy-ingress/pkg/haproxy/instance.go

Line 661 in 914b581

if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {

@jcmoraisjr
Copy link
Owner Author

Hi, in fact this shouldn't make any difference because, when shutdown.sh is called, the controller process is being shutting down as well, dropping all the other process in that same container. So moving from term to usr1 would be a noop to the observer. I understand your concern, and I also think we've already discussed that on another issue, and the point here is that the correct place to configure a long living haproxy connection during rolling updates is not here, but instead making the controller to postpone its own shutdown. Moreover, this script belongs to a way to run haproxy which is being deprecated. master-worker is the new default from v0.15, and it calls haproxy directly without any help of external scripts.

@avmnu-sng
Copy link

@jcmoraisjr Yeah, we discussed it earlier. I guess I understand now why USR1 is still a no-op. For example, suppose we have wait-before-shutdown=10s. Then, after 10s, irrespective of USR1 or TERM on HAProxy, the ingress controller exits immediately. It would only work if we wait after triggering the HAProxy shutdown. Even if we move to master-worker, we still need a soft stop on HAProxy, and shouldn't ideally, the controller should exit after HAProxy shutdown completes.

@jcmoraisjr
Copy link
Owner Author

I see your point. When running haproxy as a daemon we don't have a good control of the underlying process. In master worker mode we run it in the foreground, this gives us a few more options.

Here is where the process is started, stopped, and asked to be stopped:

haproxy-ingress/pkg/haproxy/instance.go

Lines 632 to 667 in a7e4b1f

func (i *instance) startHAProxySync() {
cmd := exec.Command(
"haproxy",
"-W",
"-S", i.options.MasterSocket+",mode,600",
"-f", i.options.HAProxyCfgDir)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
if err := cmd.Start(); err != nil {
i.logger.Error("error starting haproxy: %v", err)
return
}
wait := make(chan struct{})
go func() {
if err := cmd.Wait(); err != nil {
if exitError, ok := err.(*exec.ExitError); ok {
i.logger.Info("haproxy stopped (exit code: %d)", exitError.ExitCode())
} else {
i.logger.Error("error while running haproxy: %v", err)
}
} else {
i.logger.Info("haproxy stopped")
}
close(wait)
}()
select {
case <-i.options.StopCh:
i.logger.Info("stopping haproxy master process (pid: %d)", cmd.Process.Pid)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
i.logger.Error("error stopping haproxy process: %v", err)
}
<-wait
case <-wait:
}
}

and here is the channel that makes the controller wait until haproxy is gone:

haproxy-ingress/pkg/haproxy/instance.go

Lines 402 to 423 in a7e4b1f

func (i *instance) Shutdown() {
if !i.up || i.options.IsExternal {
// lifecycle isn't controlled by HAProxy Ingress
return
}
if i.options.IsMasterWorker {
<-i.waitProc
return
}
i.logger.Info("shutting down embedded haproxy")
out, err := exec.Command(
i.options.RootFSPrefix+"/haproxy-shutdown.sh",
i.options.LocalFSPrefix,
).CombinedOutput()
outstr := string(out)
if outstr != "" {
i.logger.Warn("output from the shutdown process: %v", outstr)
}
if err != nil {
i.logger.Error("error shutting down haproxy: %v", err)
}
}

In this scenario it makes sense to send usr1 because the controller will wait for it. I'll give this a chance, maybe adding as a config option. Tks btw for bringing this scenario again and again =)

@avmnu-sng
Copy link

@jcmoraisjr No worries; finally, we are on the same page. For now, with Setpgid: true, running in master-worker mode with a sensible wait-before-shutdown value would take us closer to a soft-stop experience.

Please give the config option approach a shot; it would be helpful. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
v0.14 v0.15
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcmoraisjr @avmnu-sng